Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: emit a warning when no content is rendered #231

Merged
merged 3 commits into from
Apr 18, 2022

Conversation

miketheman
Copy link
Member

Fixes #149

Signed-off-by: Mike Fiedler miketheman@gmail.com

@miketheman
Copy link
Member Author

cc @bhrutledge - the bug surfaced via a twine check command - see original issue.
This library hadn't previously emitted any specific warnings, rather passing through any received from docutils, so this is the first that I can tell where we're emitting our own to the stream.

@bhrutledge
Copy link
Contributor

@miketheman Thanks for sharing, and fixing! I'd like to take a look at how this displays in Twine, and see if there could be a more helpful message. I should be able to do that later today.

@bhrutledge bhrutledge self-requested a review March 13, 2022 16:39
@@ -129,4 +129,5 @@ def render(
if rendered:
return clean(rendered)
else:
stream.write("no output rendered")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could use some more work. Given a README.rst like:

Header
======

The latest (unreleased) version of Twine shows:

Screen Shot 2022-03-13 at 9 17 31 PM

First, it'd be nice if the no output rendered message were more helpful, e.g. with a suggested resolution.

Furthermore, this new output also shows up for RST syntax errors. With a spurious header line:

Header
======

======

The output is:

Screen Shot 2022-03-13 at 9 17 02 PM

Compared to readme_renderer==34.0:

Screen Shot 2022-03-13 at 9 15 58 PM


Also, I wonder if the scenario described in #149 (automatically removed header) could/should be treated the same way as an empty README.rst, i.e., passing with an empty description:

Screen Shot 2022-03-13 at 9 15 47 PM

Copy link
Member Author

@miketheman miketheman Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bhrutledge Thanks for testing this branch - these examples are very helpful!

Considering that the condition expressed in the added test case is intentional behavior, would it make sense to not apply this change, and rather change how twine check behaves, rather than this library?
Maybe around https://github.com/pypa/twine/blob/a0ba32dcd0ea5af5de7b88d6645b7743bf003760/twine/commands/check.py#L102-L103 - check if there are warnings on the stream as well? This would then preserve the desired behavior of readme_renderer to emit the warnings from the docutils dependency it uses.

Then the second example with the spurious header line would still work as it's supposed to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Twine could check the warning stream, but if I follow this code, so could readme_renderer, and add a helpful warning if there's no content. I think I have a preference for the latter, to make this transparent to Twine, and useful in other projects that use this package. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough - I figured if the original report was expressing dissatisfaction with their twine check results, maybe twine would answer their need. 😉

I'll give it a shot - checking the stream for any warnings from docutils first, and if there are, leave them be, but if there are none, then add our warning.

Do you have any suggestions for a better warning message?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like "No content rendered from RST source.\n"? That's still a little vague/awkward, but at this point in the code we don't where the actual raw content is coming from. In Twine, it's not much better; it's just reading the description from the package metadata.

Considering that the condition expressed in the added test case is intentional behavior

I dug into this, and discovered some interesting behavior. Given two packages, one with an empty README.rst, and one with only a header:

% tail dist/example-rst-empty-0.0.5/PKG-INFO 
Description-Content-Type: text/x-rst
License-File: LICENSE

UNKNOWN

% tail dist/example-rst-header-0.0.5/PKG-INFO
Description-Content-Type: text/x-rst
License-File: LICENSE

Header
======

Twine will pass empty, but fail header:

% twine check dist/*.gz
Checking dist/example-rst-empty-0.0.5.tar.gz: PASSED
Checking dist/example-rst-header-0.0.5.tar.gz: FAILED
ERROR    `long_description` has syntax errors in markup and would not be rendered on PyPI.                                                
         No content rendered from RST source.

However, via some print debugging, it seems readme_renderer treats both as an error:

% python3 -m readme_renderer dist/example-rst-empty-0.0.5/README.rst; echo Exit: $? 
No content rendered from RST source.
raw = ''
Exit: 1

% python3 -m readme_renderer dist/example-rst-header-0.0.5/README.rst; echo Exit: $?
No content rendered from RST source.
raw = 'Header\n======\n'
Exit: 1

Looking at the Twine source, I think this actually exposes a bug. It seems that there’s a check for an UNKNOWN description, but with 3 trailing newlines, while the metadata only has 2. That results in a call to render(), and a “valid” result:

% twine check dist/example-rst-empty-0.0.5.tar.gz           
Checking dist/example-rst-empty-0.0.5.tar.gz: 
description = 'UNKNOWN\n\n', rendering_result = '<p>UNKNOWN</p>\n', is_ok = True
PASSED

I don't know when this behavior changed in Twine, but I think it should be fixed. For now, with a quick hack, here’s the result:

% twine check dist/*.gz     
Checking dist/example-rst-empty-0.0.5.tar.gz: 
description = 'UNKNOWN\n\n', rendering_result = None, is_ok = True
PASSED with warnings
WARNING  `long_description` missing.                                                                                                      
Checking dist/example-rst-header-0.0.5.tar.gz: 
description = 'Header\n======\n\n\n', rendering_result = None, is_ok = False
FAILED
ERROR    `long_description` has syntax errors in markup and would not be rendered on PyPI.                                                
         No content rendered from RST source.                                                                                             
         raw = 'Header\n======\n\n\n'

That looks right to me: empty generates a warning, but header is an error because the long_description won't be rendered as-is.

Before I went down this rabbit hole, I was wondering if readme_renderer should treat an empty RST file differently from one with only a header. However, from Twine's perspective, I don't think it matters. I do still think that it would be nice for readme_renderer to add a warning.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bhrutledge Thanks for digging in - super helpful!
I've pushed another commit (after rebase of the original one on the current main) - which implements the clearer message you suggested, and added some smarter detection of whether or not to emit a warning.

Glad that we exposed an interesting bug on twine as well - but considering that with this change readme_renderer will emit a warning for what it considers "empty" (after removing the first Header), does the UNKNOWN check still make sense over there? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with this change readme_renderer will emit a warning for what it considers "empty" (after removing the first Header), does the UNKNOWN check still make sense over there? 🤔

I want to look into this a little more, but I think so, because otherwise readme_renderer will see UNKNOWN\n\n, and return <p>UNKNOWN</p\n. What's not clear to me at the moment is how that's rendered on PyPI, and the whole point of twine check is to avoid surprises in PyPI rendering.

readme_renderer/rst.py Outdated Show resolved Hide resolved
readme_renderer/rst.py Outdated Show resolved Hide resolved
readme_renderer/rst.py Outdated Show resolved Hide resolved
Fixes pypa#149

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
This is a bit more gymnastics than I think is warranted,
but wanted to play out the example.

Needing to inspect the stream proved more complex than
originally expected, as each version confroms to a slightly
different interface.

- Updated the outpout string to be clearer
- Added test for empty RST file

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@miketheman miketheman marked this pull request as draft March 23, 2022 18:02
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
bhrutledge added a commit to bhrutledge/twine that referenced this pull request Mar 26, 2022
This is responding to a change in readme_renderer.rst.render that
expects an `IO[str]` instance. See discussion at
pypa/readme_renderer#231 (review)
bhrutledge added a commit to bhrutledge/twine that referenced this pull request Mar 26, 2022
A missing `long_description` results in `UNKNOWN` being written to
PKG-INFO, but at some point, the number of trailing newlines changed.

See
pypa/readme_renderer#231 (comment)
bhrutledge added a commit to bhrutledge/twine that referenced this pull request Mar 26, 2022
A missing `long_description` results in `UNKNOWN` being written to
PKG-INFO, but at some point, the number of trailing newlines changed.

See
pypa/readme_renderer#231 (comment)
bhrutledge added a commit to pypa/twine that referenced this pull request Mar 31, 2022
This is responding to a change in readme_renderer.rst.render that
expects an `IO[str]` instance. See discussion at
pypa/readme_renderer#231 (review)
bhrutledge added a commit to pypa/twine that referenced this pull request Mar 31, 2022
* Make missing long_description check more flexible.

A missing `long_description` results in `UNKNOWN` being written to
PKG-INFO, but at some point, the number of trailing newlines changed.

See
pypa/readme_renderer#231 (comment)

* Rewrite tests using `build`

* Attempt to detect missing description on Windows

* Reconfigure output before each test

* Add changelong entry
@miketheman miketheman marked this pull request as ready for review March 31, 2022 13:02
@miketheman
Copy link
Member Author

@bhrutledge I think this is now ready for re-review and release, considering that twine 4.0.0 is out with the changes to the stream.

@bhrutledge
Copy link
Contributor

@miketheman Thanks for the nudge. It's been on my list, blocked by "real life". 😉 Hopefully this weekend.

Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good! Here's the result from an empty README and a README with only a header:

% twine check dist/*.gz 
Checking dist/example-rst-empty-0.0.5.tar.gz: PASSED with warnings
WARNING  `long_description` missing.                                                                                                                                                             
Checking dist/example-rst-header-0.0.5.tar.gz: FAILED
ERROR    `long_description` has syntax errors in markup and would not be rendered on PyPI.                                                                                                       
         No content rendered from RST source.  

Comment on lines +132 to +133
# If the warnings stream is empty, docutils had none, so add ours.
if not stream.tell():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever!

@miketheman
Copy link
Member Author

Thanks! @di all yours now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RST cannot contain header only - cause silent crash
3 participants